Generic node description #230

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Merged

adam-fowler merged 4 commits into main from generic-node-description

Oct 9, 2025

Collaborator

adam-fowler commented Sep 25, 2025 •

edited

Loading

Add associatedtype NodeDescription to ValkeyNodeConnectionPool.
Add update ValkeyRunningClientsStateMachine to use NodeDescription
Add cluster specific ValkeyClusterNodeClientFactory which uses ValkeyNodeDescription as a node description

This change is so we can use ValkeyRunningClientsStateMachine in ValkeyClient which requires a different node address type.

github-actions bot commented Sep 25, 2025 •

edited

Loading

✅ Pull request no significant performance differences ✅

Summary

New baseline 'pull_request' is WITHIN the 'main' baseline thresholds.

Full Benchmark Comparison

Comparing results between 'main' and 'pull_request'

Host '2da3fdfa22c5' with 4 'x86_64' processors with 15 GB memory, running:
#18~24.04.1-Ubuntu SMP Sat Jun 28 04:46:03 UTC 2025

ValkeyBenchmarks

Client: GET benchmark metrics

Malloc (total): results within specified thresholds, fold down for details.

Malloc (total) *	p0	p25	p50	p75	p90	p99	p100	Samples
main	83	85	85	87	88	88	88	6
pull_request	79	81	85	86	87	87	87	6
Δ	-4	-4	0	-1	-1	-1	-1	0
Improvement %	5	5	0	1	1	1	1	0

Client: GET benchmark | parallel 20 | 20 concurrent connections metrics

Malloc (total): results within specified thresholds, fold down for details.

Malloc (total) *	p0	p25	p50	p75	p90	p99	p100	Samples
main	97	100	104	109	113	115	115	24
pull_request	94	101	103	106	109	113	113	25
Δ	-3	1	-1	-3	-4	-2	-2	1
Improvement %	3	-1	1	3	4	2	2	1

Connection: GET benchmark metrics

Malloc (total): results within specified thresholds, fold down for details.

Malloc (total) *	p0	p25	p50	p75	p90	p99	p100	Samples
main	4	4	4	4	4	4	4	8
pull_request	4	4	4	4	4	4	4	8
Δ	0	0	0	0	0	0	0	0
Improvement %	0	0	0	0	0	0	0	0

Connection: GET benchmark – NoOpTracer metrics

Malloc (total): results within specified thresholds, fold down for details.

Malloc (total) *	p0	p25	p50	p75	p90	p99	p100	Samples
main	7	8	8	10	10	10	10	8
pull_request	7	8	8	10	10	10	10	8
Δ	0	0	0	0	0	0	0	0
Improvement %	0	0	0	0	0	0	0	0

Connection: Pipeline array benchmark metrics

Malloc (total): results within specified thresholds, fold down for details.

Malloc (total) *	p0	p25	p50	p75	p90	p99	p100	Samples
main	33	33	34	34	34	34	34	6
pull_request	33	33	34	34	34	34	34	6
Δ	0	0	0	0	0	0	0	0
Improvement %	0	0	0	0	0	0	0	0

Connection: Pipeline benchmark metrics

Malloc (total): results within specified thresholds, fold down for details.

Malloc (total) *	p0	p25	p50	p75	p90	p99	p100	Samples
main	33	33	33	34	34	34	34	6
pull_request	33	33	33	34	34	34	34	6
Δ	0	0	0	0	0	0	0	0
Improvement %	0	0	0	0	0	0	0	0

HashSlot – {user}.whatever metrics

Malloc (total): results within specified thresholds, fold down for details.

Malloc (total) *	p0	p25	p50	p75	p90	p99	p100	Samples
main	0	0	0	0	0	0	0	18
pull_request	0	0	0	0	0	0	0	18
Δ	0	0	0	0	0	0	0	0
Improvement %	0	0	0	0	0	0	0	0

ValkeyCommandEncoder – Command with 7 words metrics

Malloc (total): results within specified thresholds, fold down for details.

Malloc (total) *	p0	p25	p50	p75	p90	p99	p100	Samples
main	0	0	0	0	0	0	0	739
pull_request	0	0	0	0	0	0	0	747
Δ	0	0	0	0	0	0	0	8
Improvement %	0	0	0	0	0	0	0	8

ValkeyCommandEncoder – Simple GET metrics

Malloc (total): results within specified thresholds, fold down for details.

Malloc (total) *	p0	p25	p50	p75	p90	p99	p100	Samples
main	0	0	0	0	0	0	0	1884
pull_request	0	0	0	0	0	0	0	1890
Δ	0	0	0	0	0	0	0	6
Improvement %	0	0	0	0	0	0	0	6

ValkeyCommandEncoder – Simple MGET 15 keys metrics

Malloc (total): results within specified thresholds, fold down for details.

Malloc (total) *	p0	p25	p50	p75	p90	p99	p100	Samples
main	0	0	0	0	0	0	0	358
pull_request	0	0	0	0	0	0	0	360
Δ	0	0	0	0	0	0	0	2
Improvement %	0	0	0	0	0	0	0	2

adam-fowler mentioned this pull request

Multiple types defining a Valkey node network address #227

Closed

nilanshu-sharma reviewed

View reviewed changes

Collaborator

nilanshu-sharma left a comment

Added comments around return types.

Sources/Valkey/Cluster/ValkeyClusterNodeClientFactory.swift

    
                  /// - Parameter nodeDescription: Description of the node to connect to.

                  /// - Returns: A configured `ValkeyNode` instance ready to connect to the specified node.

                  @usableFromInline

                  package func makeConnectionPool(nodeDescription: ValkeyNodeDescription) -> ValkeyNodeClient {

Collaborator

nilanshu-sharma Oct 7, 2025

Do you intend to return ConnectionPool here ?
package typealias ConnectionPool = ValkeyNodeClient is defined, but ConnectionPool is not being used in the file.

Collaborator Author

adam-fowler Oct 8, 2025

This type conforms to the protocol ValkeyNodeConnectionPoolFactory. This protocol has an associatedtype ConnectionPool. When you create a concrete type that conforms to a protocol with an asssociatedtype you need to indicate what the associated type is. In this case ConnectionPool is ValkeyNodeClient.

You can do this by defining the protocol requirements using that type or add a typealias. We have ended up doing both here. There is no harm in this but yeah I guess it does look a little weird.

Collaborator Author

adam-fowler Oct 8, 2025

I removed the typealias because it isn't necessary

Sources/Valkey/Node/ValkeyNodeClientFactory.swift

    
                  /// - Returns: A configured `ValkeyNode` instance ready to connect to the specified node.

                  @usableFromInline

                  package func makeConnectionPool(serverAddress: ValkeyServerAddress) -> ValkeyNodeClient {

                  package func makeConnectionPool(nodeDescription: ValkeyServerAddress) -> ValkeyNodeClient {

Collaborator

nilanshu-sharma Oct 7, 2025

Do you intend to return ConnectionPool here ?

Collaborator Author

adam-fowler Oct 8, 2025

See above

adam-fowler added 3 commits

October 8, 2025 09:32


          Add associatedtype NodeDescription to ValkeyNodeConnectionPoolFactory

260878e

This allows us have separate node descriptions for different implementations of ValkeyRunningClientsStateMachine

Signed-off-by: Adam Fowler <[email protected]>


          Add separate ValkeyNodeConnectionPoolFactory for cluster and standalone

184813a

Signed-off-by: Adam Fowler <[email protected]>


          Remove ConnectionPool typealias

3a59e06

Signed-off-by: Adam Fowler <[email protected]>

adam-fowler force-pushed the generic-node-description branch from 169f245 to 3a59e06 Compare

October 8, 2025 09:28


          Merge branch 'main' into generic-node-description

361ee8a

nilanshu-sharma approved these changes

View reviewed changes

adam-fowler merged commit 4f72314 into main

12 of 13 checks passed

adam-fowler deleted the generic-node-description branch

October 9, 2025 06:35

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet